[SYCL] optimize enqueueImpKernel by making trace faster #20682

lslusarczyk · 2025-11-19T09:35:20Z

Part of #20668 related to making tracing kernels shorter.

Core optimization is based on making checking that traces are disabled as fast & short as possible by

use __builtin_expect indicating common path
putting all code of non-common path in a separate not inlined function (in cpp)
remove "bool ResetCache" parameter and not handling its logic on common path

So inlined check is now just "if" and one function call branch which is a small instruction set and moreover should be optimized out by compiler to non-common path of instructions predicted.

    if (__builtin_expect(SYCLConfigTrace::isTraceInMemCache(), false))
      traceKernelImpl(Msg, KernelName, isFastKernelCache);

By chance I did some refactor

SYCLConfig<SYCL_CACHE_TRACE> is a specialized class but code does not utilize fact that it is a template, that is SyclConfig is never called with generic type which evaluates to SYCL_CACHE_TRACE. Therefore this specialization should be a simple class. This eases coding stuff in it.

vinser52 · 2025-11-19T15:30:55Z

sycl/source/detail/config.hpp

 // not a valid number, the disk cache tracing will be enabled (depreciated
 // behavior). The default value is 0 and no tracing is enabled.
-template <> class SYCLConfig<SYCL_CACHE_TRACE> {
+class SYCLConfigTrace {


As I understand, the main point of optimization is that you removed from the hot path the check if traces are enabled because you changed the reset logic. But why did you replace the template specialization with a new SYCLConfigTrace class? Why not apply this optimization to the SYCLConfig template class, so that it is enabled for all configs?

I have seen only this one having impact on flamegraphs. You are righ - it is worth to look at other ones and try to unify approach. Changing pattern for others may be a next step. I just wanted to save time now and focus on hunting next perf bottlenecks.

SYCLConfig template class was already specialized for traces. It does not use common code from SYCLConfig. Maybe it is possible to unify this code also for all other config values. Maybe it is not - for some reason we had this specialization. I did not analyzed the rest of variables deeply.

It sounds like you suggest accumulating the tech debt in favor of performance.
So, since we already achieved the initial perf targets the pressure from our customers is decreased, so we should look for clean solution instead of intermediate steps.

In that particular case, what was the reason to get rid of template specialization?

It sounds like you suggest accumulating the tech debt in favor of performance.

I do not introduce tech debt. I do not optimize all the code. Just the most important one. I don't do premature optimization.

In that particular case, what was the reason to get rid of template specialization?

Because this particular template specialization is useless. The simpler code, the better. A class without a template is simpler than templated class.

Because this particular template specialization is useless. The simpler code, the better. A class without a template is simpler than templated class.

This is exactly my concern: this PR introduces a discrepancy. So far, we have a unified design (maybe not the best) with template specializations for a particular config. Now we have a dedicated class (perhaps it is a simpler approach and more readable) for a particular config.

I do not introduce tech debt. I do not optimize all the code. Just the most important one. I don't do premature optimization.

I agree that we should not do premature optimizations. But in that particular case I just suggest that we need to unify the design for all config types.

I can change all specialization to concrete classes. I like simplifying the code. Will that be OK for now?

vinser52 · 2025-11-19T15:35:27Z

sycl/source/detail/persistent_device_code_cache.hpp

  static void trace(const std::string &msg, const std::string &path = "") {
-    static const bool traceEnabled =
-        SYCLConfig<SYCL_CACHE_TRACE>::isTraceDiskCache();
+    static const bool traceEnabled = SYCLConfigTrace::isTraceDiskCache();


With your new approach, when the hot path for getting the config value is optimized do we really need this static variable? Can we just do:

if (SYCLConfigTrace::isTraceDiskCache()) { ... }

good point, and with _builtin_expect, fixed in all 3 places

aelovikov-intel · 2025-11-19T17:12:51Z

PR description should describe what exact optimizations your performed.

It looks to me that at least one of them is to remove if (needsReset) condition because that only happens in unittests, so we can move the reset into a separate internal API. Is that correct? Is there anything else done?

lslusarczyk · 2025-11-20T09:35:04Z

PR description should describe what exact optimizations your performed.

Right. I have updated review description.

It looks to me that at least one of them is to remove if (needsReset) condition because that only happens in unittests, so we can move the reset into a separate internal API. Is that correct? Is there anything else done?

It was this one and a bit more - I've described in PR description.

lslusarczyk · 2025-11-20T14:39:21Z

@vinser52 , @aelovikov-intel , @sergey-semenov , have I addressed/answered your concerns correctly? If so, could you please approve and merge?
There is one fail in CI related to hung test on one windows machine, seems not to be related to my change.

aelovikov-intel · 2025-11-20T16:14:54Z

@vinser52 , @aelovikov-intel , @sergey-semenov , have I addressed/answered your concerns correctly? If so, could you please approve and merge? There is one fail in CI related to hung test on one windows machine, seems not to be related to my change.

Description is 👍 now, thanks! I'll leave code review to Sergey.

sergey-semenov

LGTM assuming the question of SYCL config class consistency gets resolved.

[SYCL] optimize enqueueImpKernel by making trace faster

c3ec15a

lslusarczyk had a problem deploying to WindowsCILock November 19, 2025 09:35 — with GitHub Actions Error

lslusarczyk force-pushed the enqueueImpKernel_trace branch from 08e1487 to c3ec15a Compare November 19, 2025 09:35

lslusarczyk had a problem deploying to WindowsCILock November 19, 2025 09:36 — with GitHub Actions Error

lslusarczyk marked this pull request as ready for review November 19, 2025 12:22

lslusarczyk requested a review from a team as a code owner November 19, 2025 12:22

lslusarczyk requested a review from aelovikov-intel November 19, 2025 12:22

lslusarczyk temporarily deployed to WindowsCILock November 19, 2025 12:22 — with GitHub Actions Inactive

lslusarczyk temporarily deployed to WindowsCILock November 19, 2025 12:55 — with GitHub Actions Inactive

lslusarczyk mentioned this pull request Nov 19, 2025

optimize enqueueImpKernel #20668

Closed

vinser52 reviewed Nov 19, 2025

View reviewed changes

aelovikov-intel requested a review from sergey-semenov November 19, 2025 17:07

applied comments

f0ee38a

lslusarczyk had a problem deploying to WindowsCILock November 20, 2025 09:33 — with GitHub Actions Failure

lslusarczyk temporarily deployed to WindowsCILock November 20, 2025 10:05 — with GitHub Actions Inactive

lslusarczyk had a problem deploying to WindowsCILock November 20, 2025 10:05 — with GitHub Actions Failure

lslusarczyk temporarily deployed to WindowsCILock November 20, 2025 12:48 — with GitHub Actions Inactive

lslusarczyk temporarily deployed to WindowsCILock November 20, 2025 13:20 — with GitHub Actions Inactive

lslusarczyk had a problem deploying to WindowsCILock November 20, 2025 13:20 — with GitHub Actions Failure

lslusarczyk had a problem deploying to WindowsCILock November 20, 2025 13:55 — with GitHub Actions Failure

lslusarczyk had a problem deploying to WindowsCILock November 20, 2025 14:50 — with GitHub Actions Failure

sergey-semenov reviewed Nov 20, 2025

View reviewed changes

lslusarczyk had a problem deploying to WindowsCILock November 20, 2025 19:55 — with GitHub Actions Failure

[SYCL] optimize enqueueImpKernel by making trace faster #20682

Are you sure you want to change the base?

[SYCL] optimize enqueueImpKernel by making trace faster #20682

Conversation

lslusarczyk commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel commented Nov 19, 2025

Uh oh!

lslusarczyk commented Nov 20, 2025

Uh oh!

lslusarczyk commented Nov 20, 2025

Uh oh!

aelovikov-intel commented Nov 20, 2025

Uh oh!

sergey-semenov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lslusarczyk commented Nov 19, 2025 •

edited

Loading